refactor(auth): replace pyOpenSSL with standard ssl and cryptography#16976
refactor(auth): replace pyOpenSSL with standard ssl and cryptography#16976nbayati wants to merge 1 commit intogoogleapis:mainfrom
Conversation
Replace pyOpenSSL with standard library ssl for mTLS transport and update key decryption to use cryptography library. This change also enhances security for handling private keys by: - Using Linux memfd_create for RAM-backed in-memory files to avoid writing secrets to physical storage. - Encrypting plaintext keys on-the-fly before writing to fallback temporary files on disk. - Securely wiping temporary files with null bytes before deletion.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the library to remove the pyOpenSSL dependency, migrating certificate and key management to the cryptography library and Python's standard ssl module. It introduces a secure three-tier fallback strategy for handling mTLS credentials via the new secure_cert_key_paths utility, which utilizes RAM-backed virtual files on Linux to minimize disk exposure. Review feedback suggests simplifying the manual context manager orchestration within this utility and using os.fdopen instead of os.write for more robust and idiomatic file descriptor operations.
| if sys.platform == "linux" and hasattr(os, "memfd_create"): | ||
| cm = _memfd_cert_key_paths(cert_bytes, key_bytes) | ||
| try: | ||
| cert_path, key_path = cm.__enter__() | ||
| except OSError: | ||
| pass # Fallback to Tier 3 on failure. | ||
| else: | ||
| try: | ||
| # Handle cases where path exists but might be restricted. | ||
| if (cert_path is None or os.path.exists(cert_path)) and ( | ||
| key_path is None or os.path.exists(key_path) | ||
| ): | ||
| yield cert_path or cert, key_path or key, passphrase | ||
| return | ||
| finally: | ||
| import sys | ||
|
|
||
| exc_info = sys.exc_info() | ||
| cm.__exit__( | ||
| *(exc_info if exc_info[0] is not None else (None, None, None)) | ||
| ) | ||
| # If verification failed, fall through to Tier 3. |
There was a problem hiding this comment.
The manual invocation of __enter__ and __exit__ for the _memfd_cert_key_paths context manager is unnecessarily complex and error-prone. You can achieve the same fallback logic much more cleanly using a standard with statement inside a try...except block. This ensures that the context is properly managed and closed even if exceptions occur during the yield or in the caller's block.
if sys.platform == "linux" and hasattr(os, "memfd_create"):
try:
with _memfd_cert_key_paths(cert_bytes, key_bytes) as (cert_path, key_path):
# Handle cases where path exists but might be restricted.
if (cert_path is None or os.path.exists(cert_path)) and (
key_path is None or os.path.exists(key_path)
):
yield cert_path or cert, key_path or key, passphrase
return
except OSError:
pass # Fallback to Tier 3 on failure.| if cert_bytes is not None: | ||
| # MFD_CLOEXEC prevents FD leaks to spawned subprocesses. | ||
| fd_cert = os.memfd_create("mtls_cert", os.MFD_CLOEXEC) | ||
| os.write(fd_cert, cert_bytes) |
There was a problem hiding this comment.
Using os.write directly can lead to partial writes, although unlikely with in-memory files. A more robust and "Pythonic" way to write the entire byte string to the file descriptor without closing it prematurely is to use os.fdopen with closefd=False.
| os.write(fd_cert, cert_bytes) | |
| with os.fdopen(fd_cert, "wb", closefd=False) as f: | |
| f.write(cert_bytes) |
|
|
||
| if key_bytes is not None: | ||
| fd_key = os.memfd_create("mtls_key", os.MFD_CLOEXEC) | ||
| os.write(fd_key, key_bytes) |
There was a problem hiding this comment.
Similar to the certificate write, using os.fdopen with closefd=False is safer and cleaner than a direct os.write call.
| os.write(fd_key, key_bytes) | |
| with os.fdopen(fd_key, "wb", closefd=False) as f: | |
| f.write(key_bytes) |
1 participant
Replace pyOpenSSL with standard library ssl for mTLS transport and update key decryption to use cryptography library.
This change also enhances security for handling private keys by:
Fixes #16920